Handle ROUTING_APP error response in onResponseTraceRoute#930
Conversation
Previously, receiving a ROUTING_APP packet in response to a traceroute would cause the function to attempt parsing it as a RouteDiscovery payload, resulting in a crash or silent failure followed by a timeout. This mirrors the error handling already present in onResponseTelemetry and onResponseWaypoint, but displays the specific errorReason instead of a hardcoded firmware version message, since traceroute is a diagnostic tool where the exact failure reason is more valuable. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
|
There was a problem hiding this comment.
Pull request overview
Updates Meshtastic’s Python interface traceroute response handling to properly deal with ROUTING_APP error responses, avoiding mis-parsing routing errors as RouteDiscovery payloads and preventing misleading timeout behavior.
Changes:
- Added an early check in
onResponseTraceRouteforROUTING_APPpackets to detect traceroute failures. - Prints the specific
routing.errorReasonon traceroute failure and terminates the traceroute wait cleanly by settingreceivedTraceRoute.
| if p["decoded"]["portnum"] == "ROUTING_APP": | ||
| error = p["decoded"]["routing"]["errorReason"] | ||
| print(f"Traceroute failed: {error}") | ||
| self._acknowledgment.receivedTraceRoute = True | ||
| return |
There was a problem hiding this comment.
I have added the unit test, can you check again ?
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Thanks for the review! Added a unit test in the latest commit that feeds a |
ROUTING_APP with errorReason NONE is an ACK (success), not an error. Only print failure message for non-NONE error reasons. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@NoctisSentinel I set copilot to re-review. I haven't had time to check on this PR yet but the changes look generally good to me, I just want to check for myself that the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #930 +/- ##
==========================================
+ Coverage 63.19% 63.24% +0.04%
==========================================
Files 25 25
Lines 4521 4527 +6
==========================================
+ Hits 2857 2863 +6
Misses 1664 1664
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Alright, checked on it. I'd forgotten if the ROUTING_APP response handling was limited to errorReason NONE (i.e., acks) or all ROUTING_APP responses, and if traceroutes requested those be passed to the response handler. It appears the answer is that traceroute handling doesn't request those, but it's only for actual acks. So this all seems sensible to me and I'll get this merged. Thanks for the PR! |
Summary
onResponseTraceRoutedid not handleROUTING_APPerror packets, causing it to attempt parsing them asRouteDiscoverypayloads, resulting in a crash or silent failure followed by a spurious timeout messageonResponseTraceRoutethat prints the specificerrorReasonand exits the wait loop cleanlyonResponseTelemetryandonResponseWaypointwhich show a hardcoded firmware version message, traceroute shows the actual error reason since it is a diagnostic tool where knowing the specific failure is more valuableTest plan
Traceroute failed: MAX_RETRANSMIT(or relevant error) without a spuriousTimed out waiting for traceroutemessage🤖 Generated with Claude Code